decode block rlp less often #9252
Conversation
debris
added
A0-pleasereview 🤓
| let uncle_bytes = encode_list(&s.block.uncles); | ||
| s.block.header.set_uncles_hash(keccak(&uncle_bytes)); | ||
| } | ||
| if s.block.header.receipts_root().is_zero() || s.block.header.receipts_root() == &KECCAK_NULL_RLP { |
There was a problem hiding this comment.
as mentioned before, this checks could lead to invalid block being locked, so I removed them
| @@ -537,18 +514,16 @@ impl LockedBlock { | |||
| self, | |||
| engine: &EthEngine, | |||
| seal: Vec<Bytes>, | |||
| ) -> Result<SealedBlock, (Error, LockedBlock)> { | |||
There was a problem hiding this comment.
LockedBlock returned in Err was never used
|
|
||
| enact( | ||
| block.header, | ||
| block.transactions, | ||
| view.uncles(), | ||
| block.uncles, |
There was a problem hiding this comment.
removed redundant decoding of uncles
| use verification::queue::kind::blocks::Unverified; | ||
|
|
||
| // create unverified block here so the `keccak` calculation can be cached. | ||
| let unverified = Unverified::from_rlp(bytes)?; |
There was a problem hiding this comment.
removed redundant block deserialization
| trace_time!("queue_ancient_block"); | ||
| let header: Header = ::rlp::Rlp::new(&block_bytes).val_at(0)?; |
There was a problem hiding this comment.
removed redundant header deserialization
| @@ -107,24 +107,6 @@ fn imports_good_block() { | |||
| assert!(!block.into_inner().is_empty()); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn fails_to_import_block_with_invalid_rlp() { | |||
There was a problem hiding this comment.
this test is not needed, cause import_block takes already deserialized block as a param
| let (h, number, parent) = { | ||
| let header = view!(BlockView, &block).header_view(); | ||
| (header.hash(), header.number(), header.parent_hash()) | ||
| let block = match Unverified::from_rlp(block) { |
There was a problem hiding this comment.
Block::is_good was doing full block deserialization. It's now replaced with Unverified::from_rlp
| } | ||
|
|
||
| let (h, number, parent) = { | ||
| let header = view!(BlockView, &block).header_view(); |
There was a problem hiding this comment.
removed redundant deserialization
| @@ -294,7 +294,7 @@ impl BlockCollection { | |||
| let header = view!(HeaderView, &block.header); | |||
| let block_view = Block::new_from_header_and_body(&header, &body); | |||
| drained.push(BlockAndReceipts { | |||
| block: block_view.rlp().as_raw().to_vec(), | |||
| block: block_view.into_inner(), | |||
There was a problem hiding this comment.
move data instead of copying it
ethcore/src/block.rs
Outdated
| })); | ||
| s.block.header.set_gas_used(s.block.receipts.last().map_or_else(U256::zero, |r| r.gas_used)); | ||
|
|
||
| let closed = self.close()?; |
There was a problem hiding this comment.
Just want to note that this will have one additional clone of unclosed_state (https://github.com/paritytech/parity-ethereum/blob/a19faf01f347e7f485a1e1a82c47e7715e132dba/ethcore/src/block.rs#L397), which is immediately dropped. Not sure whether it's optimized away by compiler.
There was a problem hiding this comment.
Or maybe we can do it the other way around:
pub fn close(self) -> Result<ClosedBlock, Error> {
let unclosed_state = self.block.state.clone();
let locked = self.close_and_lock()?;
Ok(ClosedBlock {
block: locked.block,
unclosed_state,
})
}
debris
added
A8-looksgood 🦄
|
|
||
| // Commit results | ||
| let mut batch = DBTransaction::new(); | ||
| chain.insert_unordered_block(&mut batch, block, receipts, None, false, true); | ||
| chain.insert_unordered_block(&mut batch, encoded::Block::new(unverified.bytes), receipts, None, false, true); |
There was a problem hiding this comment.
So where does the verification actually happen?
There was a problem hiding this comment.
4 lines above, we verify only header for old blocks. No logic has been changed here
| let header: BlockHeader = header_rlp.as_val()?; | ||
| if header.number() > sync.highest_block.unwrap_or(0) { | ||
| sync.highest_block = Some(header.number()); | ||
| let block = Unverified::from_rlp(r.at(0)?.as_raw().to_vec())?; |
There was a problem hiding this comment.
Can we avoid allocating here? We don't even know if the block is valid at this point.
There was a problem hiding this comment.
good catch!
of course we can, I will fix it in the next pr as the allocation is not new (previously it happened in https://github.com/paritytech/parity-ethereum/pull/9252/files#diff-e192cabaad094a396a522f8a2633daceL184)
There was a problem hiding this comment.
Yes, the point is that it was happening after the initial checks - currently we eagerly allocate for every incoming block.
There was a problem hiding this comment.
but these initial checks check only block header, so if someone crafted a malicious block it would cause an allocation anyway
|
just checked syncing Kovan. It looks solid, |
|
|
||
| let unclosed_state = s.block.state.clone(); | ||
|
|
||
| s.engine.on_close_block(&mut s.block)?; |
There was a problem hiding this comment.
Where does this callback happen after this change?
There was a problem hiding this comment.
| @@ -198,7 +199,7 @@ mod tests { | |||
| let sync_client = generate_dummy_client_with_spec_and_data(Spec::new_validator_multi, 0, 0, &[]); | |||
| sync_client.engine().register_client(Arc::downgrade(&sync_client) as _); | |||
| for i in 1..4 { | |||
| sync_client.import_block(client.block(BlockId::Number(i)).unwrap().into_inner()).unwrap(); | |||
| sync_client.import_block(Unverified::from_rlp(client.block(BlockId::Number(i)).unwrap().into_inner()).unwrap()).unwrap(); | |||
There was a problem hiding this comment.
This is a very long and noisy line...
| Ok(block) => block, | ||
| Err(_) => { | ||
| debug!(target: "sync", "Bad block rlp"); | ||
| bad = true; |
There was a problem hiding this comment.
This bad/ if bad {…} smells a little, especially in such a big function. How about just returning here and below with Err(BlockDownloaderImportError::Invalid);?
There was a problem hiding this comment.
@dvdplm we still need to update the number of imported blocks before returning.
| Ok(block) => block, | ||
| Err(_) => { | ||
| debug!(target: "sync", "Bad block rlp"); | ||
| bad = true; |
There was a problem hiding this comment.
@dvdplm we still need to update the number of imported blocks before returning.
…rp_sync_on_light_client * 'master' of https://github.com/paritytech/parity: Propagate transactions for next 4 blocks. (openethereum#9265) decode block rlp less often (openethereum#9252) Fix eternalities tests can_create (missing parameter) (openethereum#9270) Update ref to `parity-common` and update `seek` behaviour (openethereum#9257) Comply EIP-86 with the new definition (openethereum#9140) Check if synced when using eth_getWork (openethereum#9193) (openethereum#9210)
in total: